find bugs code check

[Zhenya Stanilovsky]

hi, igniters.
I take a little time to analyze findbugs  http://findbugs.sourceforge.net/  output from ignite-core module.
There is summary of suspicious messages:

GridIoManager

sync on conc map:

synchronized (map) {
rmv = map.remove(set.nodeId(), set);
}
---------------

GridCacheLockImpl

read without sync monitor

final int getPermits() {
return getState();
}

final synchronized void setPermits(int permits) {
setState(permits);
}

-----------------------

GridDhtPartitionFullMap

add null check

@Override public boolean equals(Object o) {
if (this == o)
return true;

GridDhtPartitionFullMap other = (GridDhtPartitionFullMap)o;

return other.nodeId.equals(nodeId) && other.updateSeq == updateSeq;
}

GridDhtPartitionMap

add null check

@Override public boolean equals(Object o) {
if (this == o)
return true;

GridDhtPartitionMap other = (GridDhtPartitionMap)o;

return other.nodeId.equals(nodeId) && other.updateSeq == updateSeq;
}

add null check

GridNearOptimisticTxPrepareFuture

@Override public boolean equals(Object o) {
MappingKey that = (MappingKey) o;

return nearEntries == that.nearEntries && nodeId.equals(that.nodeId);
}

-----------------

copy-paste:
public class GridTuple6

@Override public boolean equals(Object o) {
if (this == o)
return true;

if (!(o instanceof GridTuple5))
return false;


-------------------

not closing stream:
public class GridClientJdkMarshaller implements GridClientMarshaller {
/** ID. */
public static final byte ID = 2;

/** {@inheritDoc} */
@Override public ByteBuffer marshal(Object obj, int off) throws IOException {
GridByteArrayOutputStream bOut = new GridByteArrayOutputStream();

ObjectOutput out = new ObjectOutputStream(bOut);
plz take a look on it, thanks !

[Mmuzaf]

Hello,

I can be wrong, but looks like -

`GridIoManager` not a bug, we are checking isEmpty here.
`GridCacheLockImpl` not a bug, variable is volatile.

Suppose, null-check not too critial for these classes, but can be done.

`GridTuple6` - must be fixed.
`GridClientJdkMarshaller`  - should be fixed.

On Tue, 31 Jul 2018 at 16:44 Евгений Станиловский
<[hidden email]> wrote:

> hi, igniters.
> I take a little time to analyze findbugs  http://findbugs.sourceforge.net/
>  output from ignite-core module.
> There is summary of suspicious messages:
>
> GridIoManager
>
> sync on conc map:
>
> synchronized (map) {
> rmv = map.remove(set.nodeId(), set);
> }
> ---------------
>
> GridCacheLockImpl
>
> read without sync monitor
>
> final int getPermits() {
> return getState();
> }
>
> final synchronized void setPermits(int permits) {
> setState(permits);
> }
>
> -----------------------
>
> GridDhtPartitionFullMap
>
> add null check
>
> @Override public boolean equals(Object o) {
> if (this == o)
> return true;
>
> GridDhtPartitionFullMap other = (GridDhtPartitionFullMap)o;
>
> return other.nodeId.equals(nodeId) && other.updateSeq == updateSeq;
> }
>
> GridDhtPartitionMap
>
> add null check
>
> @Override public boolean equals(Object o) {
> if (this == o)
> return true;
>
> GridDhtPartitionMap other = (GridDhtPartitionMap)o;
>
> return other.nodeId.equals(nodeId) && other.updateSeq == updateSeq;
> }
>
> add null check
>
> GridNearOptimisticTxPrepareFuture
>
> @Override public boolean equals(Object o) {
> MappingKey that = (MappingKey) o;
>
> return nearEntries == that.nearEntries && nodeId.equals(that.nodeId);
> }
>
> -----------------
>
> copy-paste:
> public class GridTuple6
>
> @Override public boolean equals(Object o) {
> if (this == o)
> return true;
>
> if (!(o instanceof GridTuple5))
> return false;
>
>
> -------------------
>
> not closing stream:
> public class GridClientJdkMarshaller implements GridClientMarshaller {
> /** ID. */
> public static final byte ID = 2;
>
> /** {@inheritDoc} */
> @Override public ByteBuffer marshal(Object obj, int off) throws
> IOException {
> GridByteArrayOutputStream bOut = new GridByteArrayOutputStream();
>
> ObjectOutput out = new ObjectOutputStream(bOut);
> plz take a look on it, thanks !

--
--
Maxim Muzafarov

[Dmitriy Pavlov]

Hi Maxim, Evgeniy,

Thank you for researching and finding these issues.

Who could create tickets (probably, newbie) so newcomers can pick up and
fix it?

Sincerely,
Dmitriy Pavlov

ср, 1 авг. 2018 г. в 11:36, Maxim Muzafarov <[hidden email]>:

> Hello,
>
> I can be wrong, but looks like -
>
> `GridIoManager` not a bug, we are checking isEmpty here.
> `GridCacheLockImpl` not a bug, variable is volatile.
>
> Suppose, null-check not too critial for these classes, but can be done.
>
> `GridTuple6` - must be fixed.
> `GridClientJdkMarshaller`  - should be fixed.
>
> On Tue, 31 Jul 2018 at 16:44 Евгений Станиловский
> <[hidden email]> wrote:
>
> > hi, igniters.
> > I take a little time to analyze findbugs
> http://findbugs.sourceforge.net/
> >  output from ignite-core module.
> > There is summary of suspicious messages:
> >
> > GridIoManager
> >
> > sync on conc map:
> >
> > synchronized (map) {
> > rmv = map.remove(set.nodeId(), set);
> > }
> > ---------------
> >
> > GridCacheLockImpl
> >
> > read without sync monitor
> >
> > final int getPermits() {
> > return getState();
> > }
> >
> > final synchronized void setPermits(int permits) {
> > setState(permits);
> > }
> >
> > -----------------------
> >
> > GridDhtPartitionFullMap
> >
> > add null check
> >
> > @Override public boolean equals(Object o) {
> > if (this == o)
> > return true;
> >
> > GridDhtPartitionFullMap other = (GridDhtPartitionFullMap)o;
> >
> > return other.nodeId.equals(nodeId) && other.updateSeq == updateSeq;
> > }
> >
> > GridDhtPartitionMap
> >
> > add null check
> >
> > @Override public boolean equals(Object o) {
> > if (this == o)
> > return true;
> >
> > GridDhtPartitionMap other = (GridDhtPartitionMap)o;
> >
> > return other.nodeId.equals(nodeId) && other.updateSeq == updateSeq;
> > }
> >
> > add null check
> >
> > GridNearOptimisticTxPrepareFuture
> >
> > @Override public boolean equals(Object o) {
> > MappingKey that = (MappingKey) o;
> >
> > return nearEntries == that.nearEntries && nodeId.equals(that.nodeId);
> > }
> >
> > -----------------
> >
> > copy-paste:
> > public class GridTuple6
> >
> > @Override public boolean equals(Object o) {
> > if (this == o)
> > return true;
> >
> > if (!(o instanceof GridTuple5))
> > return false;
> >
> >
> > -------------------
> >
> > not closing stream:
> > public class GridClientJdkMarshaller implements GridClientMarshaller {
> > /** ID. */
> > public static final byte ID = 2;
> >
> > /** {@inheritDoc} */
> > @Override public ByteBuffer marshal(Object obj, int off) throws
> > IOException {
> > GridByteArrayOutputStream bOut = new GridByteArrayOutputStream();
> >
> > ObjectOutput out = new ObjectOutputStream(bOut);
> > plz take a look on it, thanks !
>
> --
> --
> Maxim Muzafarov
>

[Nikolai Kulagin]

Hi, igniters,

As a result of this discussion [1] two issue were created:

 - NPE and CCE on equals() methods [2];
 - Methods may fail to close stream in core module [3].

In addition to the list of bugs from the letter, FindBugs plug-in was
additionally analyzed the core module for the presence of similar problems.
Found bugs added to issues.

In the future, it is planned to expand the list, so the issues were tagged
with the FindBugs tag for easy navigation. It is also necessary to check the
remaining modules.

[1]
http://apache-ignite-developers.2346864.n4.nabble.com/find-bugs-code-check-td33261.html 
[2] https://issues.apache.org/jira/projects/IGNITE/issues/IGNITE-9160 
[3] https://issues.apache.org/jira/projects/IGNITE/issues/IGNITE-9165 



--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/

[Nikolai Kulagin]

Hi. Igniters!

I create two PR [1,2] for two issues: IGNITE-9165 (close streams) [3] and
IGNITE-9160 (fix equals methods) [4]. Can you please review it? The changes
are quite small, tests are ok, flacky as usual.

[1] https://github.com/apache/ignite/pull/4481
[2] https://github.com/apache/ignite/pull/4478
[3] https://jira.apache.org/jira/browse/IGNITE-9165
[4] https://jira.apache.org/jira/browse/IGNITE-9160



--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/